Skip to content

feat: Make settings not thread-local #1444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 102 commits into from
Oct 15, 2025
Merged

feat: Make settings not thread-local #1444

merged 102 commits into from
Oct 15, 2025

Conversation

@scouten-adobe
Copy link
Collaborator

@scouten-adobe scouten-adobe commented Sep 24, 2025 •
edited
Loading

Settings are now snapshotted at public API surfaces and passed through all layers below.

Note related work from @ok-nick in #1447 and further work likely in #1454.

@scouten-adobe
Spike: Retrofit io_utils call sites
44d8faa
@scouten-adobe
Spike: Adapt verify_cose
e509d31
@scouten-adobe
Spike: Retrofit Store::handle_remote_manifest
2a39a23
@scouten-adobe
Spike: Retrofit sign_claim
73aead9
@scouten-adobe
Spike: Push farther up the Store API chain
296aa51
@scouten-adobe
Spike: Push Settings up through Ingredient API chain
ffd8015
@scouten-adobe
Spike: Push settings up through Reader API
de83d42
@scouten-adobe
Spike: Push settings farther up Builder and Ingredient API chains
5e638e8
@scouten-adobe
Spike: Push settings up through thumbnail utils
b5093a4
@scouten-adobe
Switch check_ingredient_trust to passed settings
023fde2
@scouten-adobe
Plumb settings through core OCSP logic
427c7e1
@scouten-adobe
Plumb settings through Store's OCSP logic
d3a078f
@scouten-adobe
One more reference in ingredient
eb40a9f
@scouten-adobe
Plumb settings through add_auto_actions_assertions_settings
ae3aee6
@scouten-adobe
Plumb settings through more of Builder
929a396
@scouten-adobe
Remove last references to get_settings_value in Builder
899b0ab
@scouten-adobe
Remove remaining references to get_settings_value in Store
fbe8976
@scouten-adobe
Remove remaining references to get_settings_value in Reader
c4e255c
@scouten-adobe
Remove remaining references to get_settings_value in Claim
3a4d500
@scouten-adobe
Remove remaining references to get_settings_value from IdentityAssertion
68ba524
@scouten-adobe
Remove remaining get_settings_value references from time_stamp code
fce4aa8
@scouten-adobe
Remove remaining references to get_settings_value outside Settings mod
beecb93
@scouten-adobe
Move Settings::get_value to private
81efbdc
@scouten-adobe
Move Settings::reset to private
3da6b20
@scouten-adobe
Merge branch 'main' into scouten/settings-not-thread-local
c3de5a0
@scouten-adobe scouten-adobe self-assigned this Sep 24, 2025
@codecov
Copy link

codecov bot commented Sep 24, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 84.53608% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.96%. Comparing base (f1b19d0) to head (b7bbc73).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sdk/src/crypto/cose/ocsp.rs 28.12% 23 Missing ⚠️
sdk/src/reader.rs 69.69% 10 Missing ⚠️
sdk/src/assertions/timestamp.rs 0.00% 9 Missing ⚠️
sdk/src/builder.rs 88.73% 8 Missing ⚠️
sdk/src/claim.rs 82.75% 5 Missing ⚠️
sdk/src/assertions/bmff_hash.rs 33.33% 2 Missing ⚠️
sdk/src/identity/identity_assertion/assertion.rs 87.50% 2 Missing ⚠️
sdk/src/crypto/time_stamp/http_request.rs 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
- Coverage   78.41%   77.96%   -0.45%     
==========================================
  Files         162      162              
  Lines       39629    40196     +567     
==========================================
+ Hits        31075    31340     +265     
- Misses       8554     8856     +302

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 24, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #1444 will improve performances by 14.77%

Comparing scouten/settings-not-thread-local (b7bbc73) with main (f1b19d0)

Summary

⚡ 16 improvements
⏩ 2 skipped1

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ read 100kb-signed.gif (with manifest) 6.3 ms 4.5 ms +39.04%
⚡ read 100kb-signed.jpg (with manifest) 13.7 ms 11.9 ms +15.01%
⚡ read 100kb-signed.mp3 (with manifest) 6.7 ms 4.9 ms +35.93%
⚡ read 100kb-signed.mp4 (with manifest) 6.6 ms 4.8 ms +36.68%
⚡ read 100kb-signed.png (with manifest) 13.8 ms 12 ms +14.77%
⚡ read 100kb-signed.svg (with manifest) 10.3 ms 8.5 ms +20.72%
⚡ read 100kb-signed.tiff (with manifest) 4.5 ms 2.7 ms +63.84%
⚡ read 100kb-signed.wav (with manifest) 6.4 ms 4.7 ms +37.66%
⚡ sign 100kb gif 6.5 ms 3.9 ms +67.26%
⚡ sign 100kb jpeg 7.2 ms 5.4 ms +34.45%
⚡ sign 100kb mp3 7.1 ms 4.6 ms +55.29%
⚡ sign 100kb mp4 10 ms 5.6 ms +77.89%
⚡ sign 100kb png 7.8 ms 5.7 ms +38.7%
⚡ sign 100kb svg 19.9 ms 17.3 ms +14.83%
⚡ sign 100kb tiff 4.8 ms 2 ms ×2.4
⚡ sign 100kb wav 7.1 ms 4.7 ms +50.25%

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

ok-nick
ok-nick reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@ok-nick ok-nick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a few places in this PR we can clean up and consolidate settings, however, many of those changes would require substantial refactoring. I think we can defer that additional work to separate PRs to make this one easier to review. This PR makes settings fully pass by reference and after we address a few open questions, it should be good to go.

mauricefisher64
mauricefisher64 approved these changes Oct 15, 2025
View reviewed changes
Copy link
Collaborator

@mauricefisher64 mauricefisher64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve assuming Gavin's questions are answered.

@ok-nick ok-nick merged commit 2ffde3d into main Oct 15, 2025
43 of 44 checks passed
@ok-nick ok-nick deleted the scouten/settings-not-thread-local branch October 15, 2025 16:44
This was referenced Oct 15, 2025
Closed
Closed
@scouten-adobe scouten-adobe mentioned this pull request Oct 22, 2025
Closed
@ok-nick ok-nick mentioned this pull request Oct 23, 2025
Open
@scouten-adobe scouten-adobe mentioned this pull request Oct 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dyro dyro dyro left review comments

@tmathern tmathern tmathern left review comments

@ok-nick ok-nick ok-nick approved these changes

@mauricefisher64 mauricefisher64 mauricefisher64 approved these changes

@gpeacock gpeacock Awaiting requested review from gpeacock

Assignees

@ok-nick ok-nick

Labels

check-release Add this label to any PR to invoke a larger suite of tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@scouten-adobe @TrevorChristman @dyro @ok-nick @tmathern @mauricefisher64